Reimplement Coraza-SPOA #103
Conversation
| with: | ||
| go-version: '1.21' | ||
|
|
||
| - name: setup environment |
There was a problem hiding this comment.
I'd really avoid going in this direction, this assumes a haproxy is running in local which is going to be a big friction to those willing to contribute. Personally I would never install haproxy locally to debug a problem so I suggest we spin up a haproxy instance using docker with testcontainers api (see https://pkg.go.dev/github.com/testcontainers/testcontainers-go) or just docker-compose so the test is selfcontained and does not assume I have something installed.
There was a problem hiding this comment.
I explicitly did this without using containers (atleast inside the CI) as its faster to run. Locally and in production I would always recommend running without docker as the performance difference is huge.
There was a problem hiding this comment.
(Also I dont have docker on my machine since its just very slow on macos)
There was a problem hiding this comment.
I really prefer to have something that is portable. It is OK if we want CI to run haproxy locally but we also need to provide a way to run it with docker. Again here we prioritize user experience if I have to install locally haproxy to contribute I'd rather not contribute hence we use containers. For advanced users they can definitively use local haproxy.
| "github.com/dropmorepackets/haproxy-go/pkg/testutil" | ||
| ) | ||
|
|
||
| const directives = ` |
There was a problem hiding this comment.
Please use go:embed for this, also in general we would like to keep a consistent http e2e suite by suing https://github.com/corazawaf/coraza/tree/main/http/e2e which is being used in coraza http handler, caddy and coraza-proxy-wasm. Running it is as easy as spin up an instance and run a go command, see https://github.com/corazawaf/coraza-caddy/blob/main/magefile.go#L74
There was a problem hiding this comment.
Why should I use embed for a single string? Inside the coraza tests its done the same way.
There was a problem hiding this comment.
But yeah I can migrate the e2e tests to run the command but I am not a fan of running commands and basically ignoring the go test framework. If the Upstream would properly expose the tests as a list we could run these in parallel and reduce testing times while also reducing the amount of external dependencies
There was a problem hiding this comment.
I opened an issue on coraza itself to expose this and make it easier for other repos to use the e2e test suite.
corazawaf/coraza#1006
There was a problem hiding this comment.
Waiting for a coraza release to fix this up
There was a problem hiding this comment.
Maybe it is better you use master for now.
fionera
force-pushed
the
rewrite
branch
3 times, most recently
from
ee4e7f7
to
f0db161
Compare
|
Sorry that I didn't update on this again but I am currently working on getting a talk done. Will try to get the other points done by the end of next week |
|
I think this can be reviewed and tested now. There is still a fd leak when reloading and changing the logger, also when there is a reload with logger change and then an a reload that reverts this, the old file can be raced. There should probably a small wrapper around the logger to provide prevent multiple writers to the same file, but I guess this can wait. |
|
@fionera Can you solve the conflicts here? |
|
Sure will do that |
|
Updated :) |
This allows us to not rely on the http header, but instead use a variable inside the transaction.
This does leak fds as the logging target never gets closed, this has to be fixed before release but this will work for now.
It is not correctly configured anyway and we have go specific linters enabled
I don't know why I didn't just close it after leaving the function. There is no other use of the fd...
|
And now without missing to remove an old file 🗡️ |
|
Yeah, can we ensure this gets approved/merged? |
Another go at updating coraza-spoa to a the up to date coraza version and a faster spop library. It also uses upstream e2e tests
Q: Why rewrite instead of incremental changes?
A: I didn't had time nor motivation to figure out why some things are how they are. It was just easier to do it like this.
TODOs:
Other PRs that are similar:
Closes #84 #85 #90 #88 #75